Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: unable to use CRDs imported as modules #3897

Closed
wants to merge 12 commits into from
Closed

Conversation

9bany
Copy link
Contributor

@9bany 9bany commented May 15, 2023

Proposed changes

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@9bany 9bany requested a review from a team as a code owner May 15, 2023 07:31
@github-actions github-actions bot added dependencies Pull requests that update a dependency file documentation Pull requests/issues for documentation labels May 15, 2023
@9bany 9bany changed the title fix: unable to use CRDs imported as modules feature: unable to use CRDs imported as modules May 15, 2023
@brianehlert brianehlert linked an issue May 17, 2023 that may be closed by this pull request
@brianehlert brianehlert added this to the v3.3.0 milestone Jun 20, 2023
@jjngx
Copy link
Contributor

jjngx commented Jun 28, 2023

@9bany could you please update the PR to fix the conflict and push changes?

@jjngx
Copy link
Contributor

jjngx commented Jul 3, 2023

@9bany thank you for contributing! Great work! Could you please resolve conflict and push your changes? We would be happy to merge your PR.

Copy link
Contributor

@jjngx jjngx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 LGTM

@jjngx jjngx requested a review from a team July 3, 2023 16:15
@jjngx jjngx requested review from a team and ciarams87 July 4, 2023 08:18
@jjngx
Copy link
Contributor

jjngx commented Jul 4, 2023

@9bany could you please run make lint and gofumpt and push your updates? It seems like internal/k8s/controller.go needs to be formatted.

➜  kubernetes-ingress git:(9bany/main) make lint

// output partially omitted

internal/k8s/controller.go:27: File is not `gofmt`-ed with `-s` (gofmt)
  "github.com/nginxinc/kubernetes-ingress/v3/pkg/apis/dos/v1beta1"
	"golang.org/x/exp/maps"
INFO File cache stats: 1 entries of total size 143.7KiB
INFO Memory: 12 samples, avg is 43.7MB, max is 89.7MB
INFO Execution took 1.094010333s
make: *** [lint] Error 1

Copy link
Contributor

@jjngx jjngx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@jjngx
Copy link
Contributor

jjngx commented Jul 12, 2023

@9bany could you please update branch push your changes?

@jjngx jjngx requested a review from a team July 13, 2023 09:55
@jjngx
Copy link
Contributor

jjngx commented Jul 13, 2023

@9bany could you please open (change this PR) a PR from your fork and the branch name different than main? So, fork the project, create a branch for your fix and open PR?
It could be sth like:

NGINX:main <- 9bany:module-support

where module-support is the name of your branch

more info: https://github.com/nginxinc/kubernetes-ingress/blob/main/CONTRIBUTING.md#open-a-pull-request

@jjngx
Copy link
Contributor

jjngx commented Jul 20, 2023

@9bany any update on this? Would you be able to send 2nd PR from your branch different than main?

@jjngx jjngx added the waiting for response Waiting for author's response label Jul 20, 2023
@9bany
Copy link
Contributor Author

9bany commented Jul 21, 2023

I will fight back on Monday, thanks for your support @jjngx

@9bany 9bany deleted the branch nginx:main July 26, 2023 09:54
@9bany 9bany closed this Jul 26, 2023
@9bany 9bany deleted the main branch July 26, 2023 09:54
@9bany 9bany restored the main branch July 26, 2023 09:56
@9bany
Copy link
Contributor Author

9bany commented Jul 26, 2023

All done, please check my new pull request here @jjngx

@jjngx
Copy link
Contributor

jjngx commented Jul 26, 2023

thanks @9bany , I will start working on your PR soon.

@jjngx
Copy link
Contributor

jjngx commented Jul 27, 2023

@9bany thank you for contributing! One question, as part of your testing did you run code generators? They can be executed using the following make targets:

$ make
...
update-codegen                 Generate code
update-crds                    Update CRDs
verify-codegen                 Verify code generation
...

@9bany
Copy link
Contributor Author

9bany commented Jul 27, 2023

@jjngx
I haven't run it yet.
but
I tried to fix it, but nothing changed when I ran it.
Screenshot 2023-07-27 at 15 23 40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file documentation Pull requests/issues for documentation waiting for response Waiting for author's response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to use CRDs imported as modules
3 participants